Short-circuit comparison chains in rewritten asserts by SirHegel · Pull Request #14918 · pytest-dev/pytest

SirHegel · 2026-08-20T17:16:34Z

The problem

Python evaluates a comparison chain lazily: in a < b < c, c is never evaluated when a < b is false. visit_Compare walked the comparators in a loop, assigning every result, and only combined them with an and afterwards — by which point everything had already run.

def test_raises_the_wrong_error():
    assert 1 < 0 < 1 / 0        # rewritten: ZeroDivisionError, not AssertionError

def test_calls_what_it_should_not():
    calls = []
    def boom():
        calls.append("boom")
        return 5
    try:
        assert 1 < 0 < boom()
    except AssertionError:
        pass
    assert calls == []          # rewritten: calls == ["boom"]

Both pass under --assert=plain and fail when rewritten.

The change

Each comparison after the first is nested inside an if on the previous result — what visit_BoolOp has done for and/or since #57. For assert 1 < 0 < boom():

 @py_assert0 = 1
 @py_assert4 = 0
+@py_assert3 = @py_assert7 = None
 @py_assert2 = @py_assert0 < @py_assert4
-@py_assert7 = boom()
-@py_assert3 = @py_assert4 < @py_assert7
+if @py_assert2:
+    @py_assert7 = boom()
+    @py_assert3 = @py_assert4 < @py_assert7
 if not (@py_assert2 and @py_assert3):

The and in the guard already short-circuits, so res is safe. The explanation is not: it builds its arguments eagerly, and a skipped operand would leave an unbound name for _saferepr to read. Everything a skipped operand would have bound is set to None before the chain.

Nothing ever reads those Nones. _call_reprcompare breaks at the first falsy result and reports only that pair — which is exactly where the chain stopped, since a chain only short-circuits after a false comparison. That is also why the messages do not change: assert 1 < 3 < 5 <= 4 < 7 still reports assert 5 <= 4.

Notes for review

The None assignment is inserted after comp.left's own statements, so a compound left operand is still evaluated exactly once and in place. With assert len(v) < g() < h() the generated code keeps @py_assert2 = len(v) first and only calls h() inside the guard.
format_variables only exists when the pytest_assertion_pass hook is enabled, so it is read through getattr.

Tests

test_comparison_chain_short_circuits covers the wrong-exception case, the unwanted-call case, and that a chain which fails partway still reports the failing pair. test_comparisons and test_custom_reprcompare pin the existing messages and are unchanged.

Full suite: 4359 passed, 97 skipped, 14 xfailed.

Checklist

Include documentation when adding new features.
Include new tests or update existing tests when applicable.
If AI agents were used, they are credited in Co-authored-by commit trailers.
Create a new changelog file in the changelog directory.
Add yourself to AUTHORS in alphabetical order.

Claude (Opus 5) was used; it is credited in the Co-authored-by trailers. I read the generated code with ast.unparse before and after, and I can answer questions on any part of it.

Python evaluates a comparison chain lazily: in a < b < c, c is never evaluated when a < b is false. visit_Compare walked the comparators in a loop, assigning every result, and only combined them with an and afterwards. By then everything had already run, so a rewritten assert could call what Python would not, and could fail with an unrelated exception instead of AssertionError. Nest each comparison after the first inside an if on the previous result, which is what visit_BoolOp already does for and/or since pytest-dev#57. The explanation builds its arguments eagerly, so a skipped operand would leave an unbound name behind. Everything a skipped operand would have bound is set to None before the chain. Nothing reads those values: _call_reprcompare stops at the first false result, and a chain only short-circuits after one. Closes pytest-dev#14819. Co-authored-by: Claude <noreply@anthropic.com>

Co-authored-by: Claude <noreply@anthropic.com>

An operand can contribute statements that run only when the assertion fails: a nested boolop appends to an explanation list built in the main statement body. Nesting only the main statements left those appends running unconditionally, against a list the skipped operand never bound, so assert 1 < 0 < (a or b) raised AttributeError on None. Nest expl_stmts on the same condition. Most operands add nothing there, and an If with an empty body is not valid ast, so the empty ones are dropped afterwards, innermost first: pruning a child can empty its parent. Collect the names to pre-bind by walking the conditional blocks for Store targets, rather than tracking self.variables and format_variables by index. That picks up @py_format names too, which pop_format_context only records when the assertion_pass hook is enabled, and drops a branch that no test could reach. Co-authored-by: Claude <noreply@anthropic.com>

SirHegel · 2026-08-20T17:36:31Z

I moved this to draft for a few hours because I found a case my own change broke, and I would rather flag it than have a reviewer find it.

Nesting only the main statement body was not enough. An operand can also contribute statements that run only when the assertion fails — a nested boolop builds its explanation by appending to a list that is created in the main body. With the list creation skipped and the appends left running unconditionally:

def test_boolop_in_a_chain():
    a = b = 0
    assert 1 < 0 < (a or b)

E       AttributeError: 'NoneType' object has no attribute 'append'

That is worse than the bug being fixed. The full suite did not catch it — there was no test for a boolop inside a comparison chain — so I found it by reading the generated code with ast.unparse for a case I had not tried.

Fixed by nesting expl_stmts on the same condition, the way visit_BoolOp nests both. Two details fell out of that:

Most operands contribute nothing to the explanation, and an ast.If with an empty body is invalid, so the empty ones are dropped afterwards — innermost first, since pruning a child can empty its parent.
The names to pre-bind are now collected by walking the conditional blocks for Store targets, instead of tracking self.variables and format_variables by index. That picks up @py_format names, which pop_format_context only records when the assertion_pass hook is on, and it removes the getattr(self, "format_variables", None) branch that no test could reach — which is also what codecov/patch was complaining about.

assert 1 < 0 < (a or b) is now part of the regression test. Full suite: 4359 passed, 97 skipped, 14 xfailed. Back to ready for review.

RonnyPfannschmidt · 2026-08-20T19:23:27Z

please crosscheck if this is already part of my stack of rewrite bugfixes/enhancements

SirHegel · 2026-08-20T19:36:57Z

Sorry, I just checked and duplicated the result; I had some leftover GPT tokens and wanted to help out with computing power—I'm auditing and helping more effectively now. My apologies; you're doing great work, brother.

SirHegel · 2026-08-20T19:37:20Z

Sorry, I just checked and duplicated the result; I had some leftover GPT tokens and wanted to help out with computing power—I'm auditing and helping more effectively now. My apologies; you're doing great work, brother.

SirHegel · 2026-08-20T19:37:22Z

Cross-checked as you asked: #14822 already does this, from three weeks ago, and reaches the same shape. Closing in favour of your stack.

One thing that came out of the comparison — I left the detail on #14822 — is that a boolop in a skipped comparator raises AttributeError on that branch:

assert 1 < 0 < (a or b)   # AttributeError: 'NoneType' object has no attribute 'append'

origin/main gives assert 1 < 0 there, so it is a regression rather than something pre-existing. The cause and the fix are in that comment.

Sorry for the duplicate. I filtered issues on "no assignee, no comments" and #14819 is both, being your own — I should have looked for a PR referencing it before starting.

psf-chronographer Bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Aug 20, 2026

Annotate inner and drop an unused type: ignore

c59b94e

Co-authored-by: Claude <noreply@anthropic.com>

SirHegel marked this pull request as draft August 20, 2026 17:32

SirHegel marked this pull request as ready for review August 20, 2026 17:36

SirHegel mentioned this pull request Aug 20, 2026

fix(rewrite): short-circuit chained comparisons #14822

Draft

SirHegel closed this Aug 20, 2026

SirHegel deleted the short-circuit-comparison-chains branch August 20, 2026 22:33

Navigation Menu

Uh oh!

Uh oh!

Short-circuit comparison chains in rewritten asserts - #14918

Short-circuit comparison chains in rewritten asserts#14918
SirHegel wants to merge 3 commits into
pytest-dev:mainfrom
SirHegel:short-circuit-comparison-chains

SirHegel commented Aug 20, 2026 •

edited

Loading

Uh oh!

SirHegel commented Aug 20, 2026

Uh oh!

RonnyPfannschmidt commented Aug 20, 2026

Uh oh!

SirHegel commented Aug 20, 2026

Uh oh!

SirHegel commented Aug 20, 2026

Uh oh!

SirHegel commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

SirHegel commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The problem

The change

Notes for review

Tests

Checklist

Uh oh!

SirHegel commented Aug 20, 2026

Uh oh!

RonnyPfannschmidt commented Aug 20, 2026

Uh oh!

SirHegel commented Aug 20, 2026

Uh oh!

SirHegel commented Aug 20, 2026

Uh oh!

SirHegel commented Aug 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SirHegel commented Aug 20, 2026 •

edited

Loading